refactor(kinesis): rename fields of `KinesisOffset` and `KinesisSplit` to make everything explicit #18704

stdrc · 2024-09-25T07:34:42Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

As part of #18697, this PR revisits the kinesis source connector and makes the offset semantics explicit in KinesisOffset and KinesisSplit. Backward-compatibility is achieved by #[serde(alias = "...")].

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

stdrc · 2024-09-25T07:34:51Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @stdrc and the rest of your teammates on Graphite

tabVersion · 2024-09-25T07:44:32Z

src/connector/src/source/kinesis/enumerator/client.rs

+                next_offset: KinesisOffset::None,
+                end_offset: KinesisOffset::None,


you can see from

risingwave/src/connector/src/source/kinesis/source/reader.rs

Lines 255 to 259 in 71732b1

KinesisOffset::SequenceNumber(seq) => (

Some(seq.clone()),

None,

ShardIteratorType::AfterSequenceNumber,

),

if kinesis read from a position, in current impl, it should begin with the next one.
So next_offset does not mean next offset

AfterSequenceNumber is a way to denote the next one after the given sequence number. I don't see any problem here...

Try read this in English: The next_offset is the offset AfterSequenceNumber("abc").

And compare it with previous one: The start_position is the offset SequenceNumber("abc").

I understand you want to eliminate the concept misalignment in the sources' split def, it is good for the project.

The variable name ought to describe the value it stores, not involving how other components do with it. But here, it stores the latest successful read offset (and literally the start offset if begin with Timestamp or TrimHorizen). It is not the next offset.
Kinesis does have its own primitive to begin consumption with the offset after the given one, your proposal may not take this for granted.

Try read this in English

Besides this, I know English well and please remain calm and friendly in communications. Be professional, dude.

Try read this in English

Besides this, I know English well and please remain calm and friendly in communications. Be professional, dude.

I'm saying that the new semantics is understandable in English, while the previous one is not.

The value stored by start_position: KinesisOffset is not the sequence number, it's After(sequence number). When recovery happens, the start position is the next one after sequence number, so what start_position = SequenceNumber(abc) actually means is start_position = AfterSequenceNumber(abc). The renaming from start_position to next_offset doesn't change any semantics, it doesn't matter. The core change is start_position = SequenceNumber(abc) to start_position = AfterSequenceNumber(abc).

In your logic, if start_position is last seen offset, then the Earliest and Latest variants should be BeforeEarliest, BeforeLatest. Is it clear enough?

In your logic, if start_position is last seen offset, then the Earliest and Latest variants should be BeforeEarliest, BeforeLatest. Is it clear enough?

Using sarcasm and rhetorical questions in the workplace is not a friendly communication attitude.

Is it clear enough?

Yes, I fully understand your motivation and proposed changes.

In your logic, if start_position is last seen offset, then the Earliest and Latest variants should be BeforeEarliest, BeforeLatest. Is it clear enough?

Using sarcasm and rhetorical questions in the workplace is not a friendly communication attitude.

Hah? I'm not using any sarcasm and rhetorical question. "Is it clear enough?" just means "这么解释足够清楚了吗？".

Don't be so sensitive. I'm just trying to explain. I admit that I was a bit impatient at the very beginning of this thread. But I didn't mean to be any aggressive after that.

Yes, I fully understand your motivation and proposed changes.

Ok, then I think we have reached a consensus.

src/connector/src/source/kinesis/split.rs

…g explicit

Signed-off-by: Richard Chien <[email protected]>

xxchan

(Didn't read the discussion above) The change lgtm

Do I understand correctly:

Previous start_position looks obstacle (the "start" word). It means how other code should use it, but seems wrong.
If we want to make it more consistent with "store" site, it should be renamed to last_seen_offset. (But seems not OK, since we have "timestamp"/"latest", etc)
If we want to make it more consistent with "read" site, call it next_offset = After(offset) looks cool.

xxchan · 2024-09-25T12:15:36Z

Wait, don't next_offset and start_offset mean the same thing..? (whether "start" is inclusive or exclusive is not clear though, (and usually inclusive (so maybe next_offset mean the same as start_offset_exclusive, lol)))

So the real great invention to me is to rename the enum variant to After

stdrc · 2024-09-25T12:19:24Z

Wait, don't next_offset and start_offset mean the same thing..? (whether "start" is inclusive or exclusive is not clear though, (and usually inclusive (so maybe next_offset mean the same as start_offset_exclusive, lol)))

So the real great invention to me is to rename the enum variant to After

Exactly. As I said #18704 (comment), renaming from start_position to next_offset is just for perfection. The real change is to change Offset::SequenceNumber(abc) to Offset::AfterSequenceNumber(abc), so that variants of KinesisOffset mean the same thing but only in different types.

stdrc mentioned this pull request Sep 25, 2024

fix(kinesis): bump aws-sdk-kinesis to 1.18.0 to fix creation failure #18702

Merged

9 tasks

github-actions bot added the Invalid PR Title label Sep 25, 2024

stdrc changed the title ~~rename fields of KinesisOffset and KinesisSplit tp make everything explicit~~ refactor(kinesis): rename fields of KinesisOffset and KinesisSplit to make everything explicit Sep 25, 2024

github-actions bot added type/refactor and removed Invalid PR Title labels Sep 25, 2024

stdrc marked this pull request as ready for review September 25, 2024 07:37

stdrc requested review from shanicky, tabVersion, wenym1 and xxchan September 25, 2024 07:37

stdrc mentioned this pull request Sep 25, 2024

Tracking: correct the semantics of offset in source connectors #18697

Open

9 tasks

tabVersion reviewed Sep 25, 2024

View reviewed changes

stdrc commented Sep 25, 2024

View reviewed changes

src/connector/src/source/kinesis/split.rs Show resolved Hide resolved

stdrc requested review from fuyufjh and tabVersion September 25, 2024 09:12

Base automatically changed from rc/fix-kinesis-source-creation to main September 25, 2024 10:26

buildkite bot requested a review from a team as a code owner September 25, 2024 10:26

stdrc added 3 commits September 25, 2024 19:52

rename fields of KinesisOffset and KinesisSplit tp make everythin…

de2a0a1

…g explicit

simplify update_offset

d9fe633

Signed-off-by: Richard Chien <[email protected]>

fix check

c3961b6

Signed-off-by: Richard Chien <[email protected]>

stdrc force-pushed the rc/kinesis-offset branch from 72f8763 to c3961b6 Compare September 25, 2024 11:53

xxchan approved these changes Sep 25, 2024

View reviewed changes

stdrc added this pull request to the merge queue Sep 25, 2024

Merged via the queue into main with commit e535aa7 Sep 25, 2024
30 of 31 checks passed

stdrc deleted the rc/kinesis-offset branch September 25, 2024 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(kinesis): rename fields of `KinesisOffset` and `KinesisSplit` to make everything explicit #18704

refactor(kinesis): rename fields of `KinesisOffset` and `KinesisSplit` to make everything explicit #18704

stdrc commented Sep 25, 2024 •

edited

Loading

stdrc commented Sep 25, 2024 •

edited

Loading

tabVersion Sep 25, 2024

stdrc Sep 25, 2024

stdrc Sep 25, 2024 •

edited

Loading

tabVersion Sep 25, 2024 •

edited

Loading

stdrc Sep 25, 2024 •

edited

Loading

stdrc Sep 25, 2024

tabVersion Sep 25, 2024 •

edited

Loading

stdrc Sep 25, 2024

stdrc Sep 25, 2024

stdrc Sep 25, 2024

xxchan left a comment

xxchan commented Sep 25, 2024

stdrc commented Sep 25, 2024 •

edited

Loading

		next_offset: KinesisOffset::None,
		end_offset: KinesisOffset::None,

	KinesisOffset::SequenceNumber(seq) => (
	Some(seq.clone()),
	None,
	ShardIteratorType::AfterSequenceNumber,
	),

refactor(kinesis): rename fields of KinesisOffset and KinesisSplit to make everything explicit #18704

refactor(kinesis): rename fields of KinesisOffset and KinesisSplit to make everything explicit #18704

Conversation

stdrc commented Sep 25, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

stdrc commented Sep 25, 2024 • edited Loading

tabVersion Sep 25, 2024

Choose a reason for hiding this comment

stdrc Sep 25, 2024

Choose a reason for hiding this comment

stdrc Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

tabVersion Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

stdrc Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

stdrc Sep 25, 2024

Choose a reason for hiding this comment

tabVersion Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

stdrc Sep 25, 2024

Choose a reason for hiding this comment

stdrc Sep 25, 2024

Choose a reason for hiding this comment

stdrc Sep 25, 2024

Choose a reason for hiding this comment

xxchan left a comment

Choose a reason for hiding this comment

xxchan commented Sep 25, 2024

stdrc commented Sep 25, 2024 • edited Loading

refactor(kinesis): rename fields of `KinesisOffset` and `KinesisSplit` to make everything explicit #18704

refactor(kinesis): rename fields of `KinesisOffset` and `KinesisSplit` to make everything explicit #18704

stdrc commented Sep 25, 2024 •

edited

Loading

stdrc commented Sep 25, 2024 •

edited

Loading

stdrc Sep 25, 2024 •

edited

Loading

tabVersion Sep 25, 2024 •

edited

Loading

stdrc Sep 25, 2024 •

edited

Loading

tabVersion Sep 25, 2024 •

edited

Loading

stdrc commented Sep 25, 2024 •

edited

Loading